Support exec auto pick bin when all bin is alias #1972
Conversation
|
hi @dr-js thanks for taking the time to propose this change 😊 it might be useful for some folks - I'll wait to hear more input from the rest of the team since I don't have much context on the particularities of npx. What I would like to add to the discussion though (and I think is a relatively less known) is that npx has a syntax that allows you to invoke any command after installing pkgs (while also having available any |
darcyclarke
added
Release 7.x
|
Thanks for the reply! About the syntax, I do know most of the variation. (also made a docs PR: #1970) This PR aims to enable the usage of the shortest syntax for more package. (most scoped package may have this problem, like common Personally I prefer to use the shortest (and often also the clearest) syntax, for screen-space saving and better readability in Also I've re-done the commit to return bin name instead of bin path, after test working by editing a local |
lib/exec.js
Outdated
| const maniBin = mani.bin || {} | ||
| if (Array.from(new Set(Object.values(maniBin))).length === 1) { | ||
| const binNameList = Object.keys(maniBin) | ||
| return binNameList[0] | ||
| } |
There was a problem hiding this comment.
Side note: don't know why test coverage drop from 100% with previous code style:
const bins = Array.from(new Set(Object.values(mani.bin || {})))
if (bins.length === 1) {
return Object.keys(mani.bin || {})[0] // test coverage will report this line uncovered
}But reducing code complexity by extracting variable seems to help here.
There was a problem hiding this comment.
The reason you get an uncovered branch on that line is because you can't possibly get there if mani.bin is missing, so the mani.bin || {} will never have a falsey mani.bin and thus never hit the || {} branch.
Try running npm test -- --coverage-report=html to open up a web browser and see missing branches, just trying to go off of line numbers can be tricky for us human brains ;)
|
I'd initially been hesitant about this, since we really don't want to guess at the intent if it's not clear. But now that I take a closer look, I see that you're checking that all of the bin values are the same thing, just with different keys. So yes, in that case, I think we can reasonably just take one of them as the default. No fundamental objection from me. |
Co-authored-by: Jordan Harband <[email protected]>
|
alright, LGTM to me too 👍 I think we just need rebasing and making sure we add document this behavior somewhere in the docs 😊 |
|
Just done rebasing and added simple behavior description to |
darcyclarke
added
release: next
Some scoped package can not use the unscoped portion of package name as
binsince it's too common, like@dr-js/coreor@dr-js/node. And put the full name inbinwill break the auto bin file creation.This change should allow the package just aliasing bin to skip add bin to
npm execcommand, like:{ "bin": { "dr-js": "bin/index.js", "DR": "bin/index.js" } }TODO: If this change is OK, I can update the doc in later commit if needed.